Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: WIP start splitting 0.6 and 0.7 logic #634

Closed
wants to merge 2 commits into from

Conversation

dancoombs
Copy link
Collaborator

@dancoombs dancoombs commented Mar 6, 2024

THIS IS BEING REPLACED BY: #635

@@ -99,7 +98,7 @@ where
builder_index: u64,
pool: C,
simulator: S,
entry_point: E,
entry_point: Arc<E>,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: figure this out, we should be consistent here, we don't need these Arc as everything inside the entry point class should be clone

SimulationResult, SimulationViolation, Simulator, ViolationError,
};
use rundler_types::{
chain::ChainSpec, Entity, EntityType, EntityUpdate, EntityUpdateType, GasFees, Timestamp,
UserOperation, UserOpsPerAggregator,
chain::ChainSpec, Entity, EntityType, EntityUpdate, EntityUpdateType, GasFees, GasOverheads,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: GasOverheads should move to chain config

Comment on lines +1224 to +1226
+ op.total_verification_gas_limit()
+ op.required_pre_execution_buffer()
+ op.call_gas_limit()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: verify this change

@@ -311,7 +311,7 @@ where
index,
self.pool.clone(),
simulator,
ep.clone(),
Arc::new(ep.clone()),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: bundle proposer currently will only handle v0.6 UOs. Figure out how to share logic

@@ -244,13 +244,6 @@ impl From<MempoolError> for ProtoMempoolError {
impl From<PrecheckViolation> for ProtoPrecheckViolationError {
fn from(value: PrecheckViolation) -> Self {
match value {
PrecheckViolation::InitCodeTooShort(length) => ProtoPrecheckViolationError {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: removed these, should add check in RPC before sending to pool.

match op {
RundlerUserOperation::V0_6(op) => op.into(),
RundlerUserOperation::V0_7(_) => {
unimplemented!("V0_7 user operation is not supported")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: implement

Comment on lines +47 to +53
const ARBITRUM_NITRO_NODE_INTERFACE_ADDRESS: Address = H160([
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0xc8,
]);

const OPTIMISM_BEDROCK_GAS_ORACLE_ADDRESS: Address = H160([
0x42, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0x0F,
]);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: we should be getting this from ChainSpec

Comment on lines +57 to +62
// TODO(danc):
// - Modify this interface to take only `UserOperationV0_6` and remove the `into_v0_6` calls
// - Places that are already in a V0_6 context can use this directly
// - Implement a wrapper that takes `UserOperation` and dispatches to the correct entry point version
// based on a constructor from ChainSpec.
// - Use either version depending on the abstraction of the caller.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO

@@ -240,14 +281,119 @@ where
.context("should compute balances")?;
Ok(out.balances)
}

async fn aggregate_signatures(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved these from provider as they are entry point dependent

@@ -98,8 +120,38 @@ pub trait EntryPoint: Send + Sync + 'static {
) -> Result<ExecutionResult, String>;

/// Get the deposit info for an address
async fn get_deposit_info(&self, address: Address) -> anyhow::Result<DepositInfo>;
async fn get_deposit_info(&self, address: Address) -> anyhow::Result<DepositInfoV0_6>;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: this will need to be made generic, can't rely on the 0_6 version.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to ExecutionResult

@@ -126,7 +127,7 @@ where
.await
.map_err(|e| rpc_err(INTERNAL_ERROR_CODE, e.to_string()))?
.into_iter()
.map(|pop| pop.uo.into())
.map(|pop| UserOperationV0_6::from(pop.uo).into())
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Should be able to map these to new RPC types that can handle both formats.

@@ -64,7 +64,7 @@ impl Settings {

#[derive(Debug)]
struct EntryPointContext<P, E> {
gas_estimator: GasEstimatorImpl<P, E>,
gas_estimator: GasEstimatorV0_6<P, E>,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: need to handle multiple types of gas estimators here, based on the entry point version. Likely easiest to just use dyn on the trait.

/// Gas estimate for a user operation
#[derive(Debug, Copy, Clone, Deserialize, Serialize)]
#[serde(rename_all = "camelCase")]
pub struct GasEstimate {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: This won't work for 0.7 estimation as we need to return more fields. Rethink this interface

@@ -87,6 +116,10 @@ where
user_op: UserOperation,
max_validation_gas: u64,
) -> anyhow::Result<TypedTransaction> {
let user_op = user_op
.into_v0_6()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if there is a way we could use some generic magic on the function and implement Into for the different types of user operations. I have not thought about implementation and just spitballing but surely we would be able to use a UO generic an call into() instead of having multiple implementations

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just skimming here so dont have full context yet

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will need multiple implementations regardless because this pulls in the contract interface which is different for v0.6 and v0.7

Comment on lines +133 to +135
+ uo.total_verification_gas_limit()
+ uo.required_pre_execution_buffer()
+ uo.call_gas_limit()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: verify the logic changes in this file

Comment on lines -400 to -402
/// The init code is too short to contain a factory address.
#[display("initCode must start with a 20-byte factory address, but was only {0} bytes")]
InitCodeTooShort(usize),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: check these in RPC now.

@dancoombs dancoombs force-pushed the danc/contracts-v0_7 branch 2 times, most recently from 6c4b2f4 to f5d2c6e Compare March 29, 2024 21:12
@dancoombs dancoombs closed this Mar 29, 2024
@dancoombs dancoombs deleted the danc/uo-enum branch November 24, 2024 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants